Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Manage the ignition stub config #1792

Merged
merged 1 commit into from
Jul 10, 2020
Merged

Manage the ignition stub config #1792

merged 1 commit into from
Jul 10, 2020

Conversation

runcom
Copy link
Member

@runcom runcom commented Jun 8, 2020 •
edited
Loading

Nothing to see here yet, just code, and testing going on elsewhere

current testing plan

Related issue #683
Enhancement openshift/enhancements#368

The key points here are:

  • if you're installing you'll get the new user-data managed by the MCO, scale up continues working
  • if you're installing with ignition v3 (4.6) this keeps working and serves the new ignition stub to new machines as the code will be typed with the new ignition structs
  • if you're upgrading from pre-ign-v3 (this is gonna be the case in our release cycle so let's not bother about v2->v3 upgrade other than in this PR I guess), then your old machine(sets) will still be able to grab the ignition v2 endpoint because we don't delete the old user-data even if the code is now ignition v3 typed

What this PR does is basically:

  • we let the installer take care of installing the initial user-data secret
  • we then take over with our managed secret
  • if we are upgrading (and thus no installer at play), we just create the new (managed) secret

Signed-off-by: Antonio Murdaca [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2020
@runcom runcom force-pushed the pointerconfig branch 2 times, most recently from 84d9b4c to bcfe84b Compare June 8, 2020 16:53
@runcom
Copy link
Member Author

runcom commented Jun 8, 2020

/retest

@runcom
Copy link
Member Author

runcom commented Jun 9, 2020

/retest

@runcom runcom mentioned this pull request Jun 9, 2020
Merged
@runcom
Copy link
Member Author

runcom commented Jun 9, 2020 •
edited
Loading

This works as expected, both alone and with openshift/installer#3730

$ oc get secrets -n openshift-machine-api
NAME                                              TYPE                                  DATA   AGE
...
master-user-data-managed                          Opaque                                2      19m
worker-user-data-managed                          Opaque                                2      19m

The new user-data is created just fine from the MCO and read correctly by machine(sets) in MAO. If you create a cluster with both this PR and openshift/installer#3730 you'll get a fully functional cluster using the new managed-by-MCO user-data:

$ oc describe machine -n openshift-machine-api ci-ln-0s13scb-d5d6b-ln8c4-master-0
...
      User Data Secret:
        Name:  master-user-data-managed
...

same for machinesets

Scaling up the worker machinesets works as expected as well

I'm going to create create the enhacement and cleanup both PRs - the MCO PR has to go in first ofc.

@runcom runcom changed the title WIP Manage the ignition stub config Manage the ignition stub config Jun 9, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2020
@runcom runcom added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2020
@runcom runcom changed the title Manage the ignition stub config WIP Manage the ignition stub config Jun 10, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2020
@runcom runcom removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2020
@runcom runcom force-pushed the pointerconfig branch 2 times, most recently from af1e238 to 998e52d Compare June 11, 2020 12:36
runcom
runcom commented Jun 11, 2020
View reviewed changes
pkg/operator/sync.go Outdated Show resolved Hide resolved
@runcom
Copy link
Member Author

runcom commented Jun 11, 2020

have just confirmed on an upgraded cluster that we effectively re-generate what the installer generates so it's safe to assume we'll generate the correct ign v3 version once we upgrade 💪

@cgwalters cgwalters mentioned this pull request Jun 15, 2020
Closed
cgwalters
cgwalters approved these changes Jun 17, 2020
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me!

manifests/master.userdata_secret.yaml Outdated Show resolved Hide resolved
pkg/controller/common/helpers.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
pkg/operator/sync.go Outdated Show resolved Hide resolved
@runcom runcom force-pushed the pointerconfig branch 2 times, most recently from 4b9b879 to 5141800 Compare June 23, 2020 14:15
@runcom runcom changed the title WIP Manage the ignition stub config Manage the ignition stub config Jun 23, 2020
@runcom
Copy link
Member Author

runcom commented Jul 1, 2020 •
edited
Loading

To be clear...this will be a no-op until the installer PR lands to start using the new managed secrets?

yeah, in the upgrade scenarios this is gonna be a no-op for all 4.6-dev as machines still points to the old userdata name. When 4.6 goes out (upgrade scenario) we simply create the new userdata secret which users can manually use in machinesets and we can leverage to be used in a future update-bootimages feature.

In the installation case it's the same as we won't be using this until installer switches naming as well

does it make sense? 🤔

(I'll update the commit message also good point 👍 ) UPDATED

@runcom runcom force-pushed the pointerconfig branch 2 times, most recently from 11d9ba2 to 3eb7839 Compare July 2, 2020 09:31
@runcom
Copy link
Member Author

runcom commented Jul 2, 2020 •
edited
Loading

I've pushed a change to allow the creation of the userdata for every pool in the cluster (which I still think it's correct as Michael pointed out in the enhancement) - it shouldn't be disruptive, we just do what we always had to do 🤷‍♂️

I'll test this out again and report back quickly:

  • create an infra MCP with this PR
  • watch the userdata secret correctly created in the openshift-machine-api namespace with the correct content

it works for me 👍

@runcom
Copy link
Member Author

runcom commented Jul 2, 2020

/skip

@runcom
Copy link
Member Author

runcom commented Jul 2, 2020

/retest

@runcom
MCO: manage the user-data secret
1f52e48 
With this we let the installer take care of installing the initial user-data secret
and we then take over with our managed secret.
if we are upgrading (and thus no installer at play), we just create the new (managed) secret.

Signed-off-by: Antonio Murdaca <[email protected]>
@yuqi-zhang
Copy link
Contributor

/retest

1 similar comment
@runcom
Copy link
Member Author

runcom commented Jul 8, 2020

/retest

@runcom
Copy link
Member Author

runcom commented Jul 8, 2020

/skip

@yuqi-zhang
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [cgwalters,runcom,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@runcom runcom mentioned this pull request Jul 9, 2020
Merged
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 9, 2020 •
edited
Loading

@runcom: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 1f52e48 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-ovn-step-registry 1f52e48 link /test e2e-ovn-step-registry

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@LorbusChris
Copy link
Member

/test e2e-gcp-upgrade

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c725488 into openshift:master Jul 10, 2020
@runcom runcom deleted the pointerconfig branch July 10, 2020 07:39
@runcom runcom mentioned this pull request Sep 29, 2020
Merged
runcom added a commit to runcom/machine-config-operator that referenced this pull request Oct 1, 2020
@runcom
Revert openshift#1792
1c65355 
Signed-off-by: Antonio Murdaca <[email protected]>
openshift-merge-robot added a commit that referenced this pull request Oct 2, 2020
@openshift-merge-robot
Merge pull request #2126 from runcom/revert-pc
d2a2e4f 
Bug 1881703: Revert #1792
@hardys hardys mentioned this pull request Oct 8, 2020
Closed
@zaneb zaneb mentioned this pull request Nov 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi-zhang yuqi-zhang yuqi-zhang approved these changes

@cgwalters cgwalters cgwalters approved these changes

@ericavonb ericavonb Awaiting requested review from ericavonb

@sinnykumari sinnykumari Awaiting requested review from sinnykumari

Assignees

@yuqi-zhang yuqi-zhang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@runcom @cgwalters @yuqi-zhang @openshift-ci-robot @openshift-bot @LorbusChris @openshift-merge-robot